Pioneer DDJ-200: fix hotcue button release #3742
Conversation
|
there will not be another 2.2 release. Also it's helpful for reviewers if you a description of the issue and how you solved it. |
|
Are you sure you don't want to have version 2.2 or 2.2.4 fixed anyway? |
|
We're very close to the release of 2.3 so we decided not to do another |
|
I don't have a problem with merging it into 2.2 as long as the changes passes our 2.3 CI Linters (we already have unreleased commits in the 2.2 branch that will end up in 2.3). We can just merge 2.2 into 2.3 after merge. But as @Swiftb0y already said, there won't be a 2.2.5 release (mainly because 2.2 is completely broken on macOS) so the vast majority of users won't notice it because I doubt that they are going to compile the 2.2 branch themselves. |
| var deckNo = script.deckFromGroup(group); | ||
| midi.sendShortMsg(0x90 + deckNo - 1, 0x0B, 0x7F * | ||
| engine.getValue(vgroup, "play")); // set play LED | ||
| if (value) { // only if button pressed, not releases, i.e. value === 0 |
There was a problem hiding this comment.
I think we should early exit here:
| if (value) { // only if button pressed, not releases, i.e. value === 0 | |
| if (!value) { | |
| return; | |
| } | |
There was a problem hiding this comment.
Generally this would be an option, but in the rest of the code I was doing it this way.
So it could be a bit inconsistent and confusing if at these two places I change the coding style.
So to go with minimal changes and maximum consistency I would suggest to keep it as is.
There was a problem hiding this comment.
Another alternative solution would be to just unbind the note-off midi in the XML.
There was a problem hiding this comment.
@Swiftb0y Buttons might be used for different purposes, i.e. for layering. I don't recommend to ignore MIDI signals in the XML, which should unconditionally forward signals to JS. Except those controls that are connected directly to COs. Subsequent filtering in JS is more versatile.
There was a problem hiding this comment.
Ok, as I understand this discussion, it should be fine either way (Holzhaus or mine).
So I would like to suggest to pull it as is.
Then I will mark this conversation as resolved, to proceed with the fix.
Is this ok?
There was a problem hiding this comment.
Yes. We'd still prefer if you could rebase on 2.3 though so the newer linters run. There is no benefit in merging into 2.2.
There was a problem hiding this comment.
Yes, but Holzhaus also said it shouldn't hurt to merge it from 2.2.
Since we are talking just about just one line, the linters will run.
There was a problem hiding this comment.
Checked it out locally, pre-commit checks passed.
fkbreitl
changed the title
ronso0
changed the title
This fix prevents triggering a hotcue when a hotcue button is released.